Skip to content

Bundle local static React Native assets #16967

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
dcalhoun merged 2 commits into from
Aug 10, 2021
Merged

Bundle local static React Native assets #16967

dcalhoun merged 2 commits into from
Aug 10, 2021

Conversation

@dcalhoun
Copy link
Member

@dcalhoun dcalhoun commented Aug 2, 2021 •
edited
Loading

Now that we attempt to load local static image assets in the React Native bundle in WordPress/gutenberg#33790, we must copy those assets for them to properly show up in the WordPress iOS app build.

To test:

  1. Use an app build that does not rely upon a local React Native Metro server.
  2. Open the block editor.
  3. Tap the three-dot menu button in the top-right corner.
  4. Tap Help.
  5. Tap one of the available articles.
  6. ℹ️ Expected: Verify the images are displayed and not missing.
Screenshot of Help Image Gutenberg Help image

Regression Notes

  1. Potential unintended areas of impact
    Modifications to the build configuration could cause the build process to fail.
  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Verified the Debug build succeeded.
  3. What automated tests I added (or what prevented me from doing so)
    I do not believe this could/should be tested with automation, as build failures should be fairly obvious.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@dcalhoun dcalhoun added the Gutenberg Editing and display of Gutenberg blocks. label Aug 2, 2021
@dcalhoun dcalhoun self-assigned this Aug 2, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 2, 2021 •
edited
Loading

You can test the changes on this Pull Request by downloading it from AppCenter here with build number: 54590. IPA is available here. If you need access to this, you can ask a maintainer to add you.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 2, 2021 •
edited
Loading

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@dcalhoun
Bundle local static React Native assets
eb915c9 
Now that we are attempting to load local static image assets in the
React Native bundle, we must copy those assets for them to properly show
up in the WordPress iOS app.
@dcalhoun dcalhoun added this to the 18.1 milestone Aug 6, 2021
@dcalhoun dcalhoun force-pushed the update/bundle-local-static-react-native-assets branch from f0b6bad to eb915c9 Compare August 6, 2021 18:41
@dcalhoun dcalhoun marked this pull request as ready for review August 6, 2021 18:41
@dcalhoun dcalhoun requested a review from ceyhun August 6, 2021 18:43
ceyhun
ceyhun reviewed Aug 9, 2021
View reviewed changes
Copy link
Contributor

@ceyhun ceyhun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dcalhoun! Tested it with the WordPress target with debug config and it's working great! Do you think maybe we should add the same change to Jetpack target as well?
Also maybe we can trigger the installable builds to verify if it builds correctly on CI with release config for both targets.

@dcalhoun
Copy link
Member Author

dcalhoun commented Aug 9, 2021

Do you think maybe we should add the same change to Jetpack target as well?

I had not considered that. Based on my understanding of how the Jetpack app also consumes Gutenberg, I would imagine we should update Jetpack's target as well. I will do that. Thank you!

Also maybe we can trigger the installable builds to verify if it builds correctly on CI with release config for both targets.

Great idea. I started a CI task now to create one.

@dcalhoun
Jetpack bundles local static React Native assets
5e338d8 
Now that we are attempting to load local static image assets in the
React Native bundle, we must copy those assets for them to properly show
up in the WordPress iOS app.
@dcalhoun
Copy link
Member Author

dcalhoun commented Aug 9, 2021

@ceyhun I updated the Jetpack target configuration to match, and verified the images loaded via a build with Xcode. Additionally, I created installable builds for both WordPress and Jetpack; those are attached to this PR. Ready for another review when you have time. Thanks!

@dcalhoun dcalhoun requested a review from ceyhun August 9, 2021 19:27
@dcalhoun dcalhoun mentioned this pull request Aug 9, 2021
Merged
2 tasks
@dcalhoun dcalhoun enabled auto-merge August 10, 2021 01:12
ceyhun
ceyhun approved these changes Aug 10, 2021
View reviewed changes
Copy link
Contributor

@ceyhun ceyhun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified that both builds have the images. Thanks David!

@dcalhoun dcalhoun merged commit 4ffe7b4 into develop Aug 10, 2021
@dcalhoun dcalhoun deleted the update/bundle-local-static-react-native-assets branch August 10, 2021 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ceyhun ceyhun ceyhun approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dcalhoun dcalhoun

Labels

Gutenberg Editing and display of Gutenberg blocks.

Projects

None yet

Milestone

18.1

Development

Successfully merging this pull request may close these issues.

3 participants

@dcalhoun @ceyhun